-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-9190 Cleanup of Stats Code Warnings #6347
Conversation
This pull request introduces 1 alert when merging 2835189 into 5f7abc9 - view on LGTM.com new alerts:
|
2835189
to
245c280
Compare
This pull request introduces 1 alert when merging 463a5a1 into 5f7abc9 - view on LGTM.com new alerts:
|
...rc/integrationTest/java/org/apache/geode/internal/cache/PartitionedRegionStatsJUnitTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few typos and some unintentionally included changes. Also, could the title of the PR please be changed to reference the associated GEODE ticket and be more descriptive?
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
Outdated
Show resolved
Hide resolved
I've approved the code changes, but please also update the name of the PR to include the GEODE ticket associated with these changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @mhansonp. But I don't see a GEODE ticket for it.
✓ typos fixed
✓ a number of int stats became long stats
✓✓ final
added in lots of places
𝙓 missing GEODE ticket reference
@mhansonp echoing @DonalEvans & @dschneider-pivotal on the GEODE jira etc... might be useful context to know the reason for the changes before I review... |
No. I removed the GEODE- ticket stuff. The typos are a freebee. :) |
I added a new improvement GEODE-9190 to cover this work. |
Context: I need to do other stat adding and I kept coming across these stats files with many warnings. I finally got fed up and cleaned them up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
@@ -1392,7 +1392,7 @@ public String getRedundancyZone() { | |||
} | |||
|
|||
int getRebalancesInProgress() { | |||
return resourceManagerStats.getRebalancesInProgress(); | |||
return (int) resourceManagerStats.getRebalancesInProgress(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue:
Possible loss of precision warning due to converting a long to int
Are there alternative to changing this MBean to be long rather than int.
Please do let me know if this is not possible on this thread and I will change the review to approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this is an external interface. I have changed nothing regarding precision. I just moved the decision of precision loss down to the interface. The core of stats went away from ints a long time ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the int because I didn't want to change the external interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to do other stat adding and I kept coming across these stats files with many warnings.